Skip to content
This repository was archived by the owner on Oct 22, 2025. It is now read-only.

chore(core): add raw ws handler example#1374

Closed
NathanFlurry wants to merge 1 commit intomainfrom
10-12-chore_core_add_raw_ws_handler_example
Closed

chore(core): add raw ws handler example#1374
NathanFlurry wants to merge 1 commit intomainfrom
10-12-chore_core_add_raw_ws_handler_example

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Oct 12, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Ready Ready Preview Comment Oct 12, 2025 5:30pm

@claude
Copy link

claude bot commented Oct 12, 2025

Claude finished @NathanFlurry's task —— View job


PR Review: Raw WebSocket Handler Example

Review Progress:

  • Read project guidelines (CLAUDE.md)
  • Analyze package.json structure and dependencies
  • Review README documentation
  • Examine backend implementation
  • Review frontend React components
  • Check configuration files
  • Evaluate code quality and best practices
  • Assess security considerations
  • Check for potential bugs
  • Review test coverage
  • Provide final recommendations

Overall Assessment

This PR adds a well-structured example demonstrating raw WebSocket handling in RivetKit. The code quality is good and follows most project conventions, but there are several areas for improvement.

🔧 Issues Found

Critical Issues

  1. README Path Inconsistency (examples/raw-websocket-handler/README.md:20)
    • Installation path shows raw-websocket-handler-proxy but actual directory is raw-websocket-handler
    • Should be: cd rivetkit/examples/raw-websocket-handler

Code Quality Issues

  1. Improper Logging (examples/raw-websocket-handler/src/backend/registry.ts:63)

    • Uses console.error() instead of required logger().error()
    • Per CLAUDE.md: "Use logger() to log messages"
  2. Missing Type Safety (examples/raw-websocket-handler/src/backend/registry.ts:13,29)

    • sockets: new Set<any>() should have proper WebSocket typing
    • socket.addEventListener("message", (event: any) - any type should be avoided
  3. Missing Error Handling (examples/raw-websocket-handler/src/frontend/App.tsx:34)

    • JSON.parse(event.data) can throw but isn't wrapped in try/catch
    • Could crash the app with malformed JSON

Security Concerns

  1. No Message Validation (examples/raw-websocket-handler/src/backend/registry.ts:33)

    • Accepts any message with data.text without validation
    • Should validate message structure and content
    • Potential for XSS if messages aren't sanitized
  2. Hardcoded CORS Origin (examples/raw-websocket-handler/src/backend/server.ts:5)

    • Development-only CORS setting should be documented or made configurable

Performance Issues

  1. Memory Leak Potential (examples/raw-websocket-handler/src/backend/registry.ts:55-60)

    • No cleanup of closed WebSocket connections from the Set
    • Current code checks readyState === 1 but doesn't remove closed sockets
  2. State Persistence Called Too Frequently (examples/raw-websocket-handler/src/backend/registry.ts:42)

    • ctx.saveState({}) called on every message - could be batched or debounced

Test Coverage

  1. No Tests
    • package.json includes vitest but no test files exist
    • vitest.config.ts configured but unused
    • README mentions test coverage as a feature but none implemented

🎯 Recommendations

High Priority Fixes

  1. Fix README path inconsistency
  2. Replace console.error with logger().error
  3. Add proper WebSocket typing instead of any
  4. Add JSON parsing error handling in frontend
  5. Add basic message validation and sanitization

Medium Priority Improvements

  1. Clean up closed WebSocket connections properly
  2. Consider batching state saves for better performance
  3. Add comprehensive test suite as advertised in README
  4. Document the hardcoded CORS configuration

Code Style Suggestions

  1. Use structured logging: logger().error({ msg: "failed to process message", error: e })
  2. Add proper JSDoc comments for the onWebSocket handler
  3. Consider extracting message types to shared interfaces

💡 Positive Aspects

  • Good Structure: Clean separation of backend/frontend code
  • Proper Dependencies: Correctly uses workspace:* for internal packages
  • Configuration: Proper Turbo, TypeScript, and build configurations
  • Functionality: Demonstrates core WebSocket features well
  • Documentation: Good README structure (minus the path issue)

🔐 Security Assessment

The code has basic security considerations but needs improvement around input validation and XSS prevention. The CORS configuration is appropriate for development.


Copy link
Member Author


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • merge queue - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@graphite-app
Copy link

graphite-app bot commented Oct 12, 2025

Merge activity

  • Oct 12, 5:30 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Oct 12, 5:30 PM UTC: CI is running for this pull request on a draft pull request (#1375) due to your merge queue CI optimization settings.
  • Oct 12, 5:31 PM UTC: Merged by the Graphite merge queue via draft PR: #1375.

graphite-app bot pushed a commit that referenced this pull request Oct 12, 2025
@graphite-app graphite-app bot closed this Oct 12, 2025
@graphite-app graphite-app bot deleted the 10-12-chore_core_add_raw_ws_handler_example branch October 12, 2025 17:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant